You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Pull request is nearly complete and ready for detailed review.
The problem was that 'noct_sam' did not correctly account for the
'effective_irradiance' parameter. The original implementation always used
'poa_global' when computing the initial temperature rise, which caused:
Open-circuit modules (efficiency = 0) to ignore 'effective_irradiance'
Very low 'effective_irradiance' values to produce unrealistic cooling
Results inconsistent with SAM under certain option combinations
This PR updates 'noct_sam' so that:
'effective_irradiance' is used consistently when provided
Negative or very small irradiance values are clipped to zero
Computation now matches SAM’s documented behaviour
I updated the noct_sam function to correctly incorporate effective irradiance into the temperature model.
New tests were added to validate:
Temperature decreases with reduced effective irradiance
Open-circuit modules still respond to effective irradiance changes
Temperature never falls below ambient for extremely small irradiance
These tests pass under the main branch, confirming correct behaviour.
I don't think we want to patch noct_sam this way. My thought is that we replace poa_global with effective_irradiance in the first arg position, drop effective_irradiance=None, and do away with the POA ratio internally. That matches the SAM calculation. The SAM function also handles calculation of effective irradiance, which isn't the job of this function in pvlib.
Replacing the argument requires a deprecation. @Chirag3841 if the deprecation machinery is a bit much, OK to close this PR and thanks for attempting the fix.
@cwhanse
I tried the approach where effective_irradiance=None defaults to poa_global and emits a deprecation warning otherwise.
However, this caused ambiguity in the function signature and resulted in multiple failures across ModelChain, PVSystem, and tests due to mixed positional and keyword arguments.
I now understand that keeping both poa_global and effective_irradiance in the same function is not a good approach.
I will revert this logic and instead keep noct_sam aligned with SAM by using effective_irradiance as the first argument only, and handle backward compatibility via a separate deprecated wrapper function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Chirag3841
changed the title
@cwhanse
The problem was that 'noct_sam' did not correctly account for the
'effective_irradiance' parameter. The original implementation always used
'poa_global' when computing the initial temperature rise, which caused:
This PR updates 'noct_sam' so that:
New tests were added to validate:
These tests pass under the main branch, confirming correct behaviour.